Miri: non-deterministic floating point operations in `foreign_items` #143906

LorrensP-2158466 · 2025-07-13T20:52:02Z

Part of rust-lang/miri/#3555, this pr does the foreign_items work.

Some things have changed since #138062 and #142514. I moved the "helpers" used for creating fixed outputs and clamping operations to their defined ranges to math.rs. These are now also extended to handle the floating-point operations in foreign_items. Tests in miri/tests/float.rs were changed/added.

Failing tests in std were extracted, run under miri with -Zmiri-many-seeds=0..1000 and changed accordingly. Double checked with -Zmiri-many-seeds.

I noticed that the C standard doesn't specify the output ranges for all of its mathematical operations; it just specifies them as:

Returns
The sinh functions return sinh x.

So I used Wolfram|Alpha.

rustbot · 2025-07-13T20:52:07Z

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2025-07-13T20:52:09Z

The Miri subtree was changed

cc @rust-lang/miri

LorrensP-2158466 · 2025-07-13T20:52:48Z

There are some holes in the behaviour of some operations, because I did not know how I could efficiently handle them, I'll mark them and add some explanation.

Also,
r? @RalfJung

src/tools/miri/src/helpers.rs

src/tools/miri/tests/pass/float.rs

src/tools/miri/src/helpers.rs

oli-obk · 2025-07-14T08:02:47Z

Please create a PR for the libstd changes on their own so a libs reviewer can review it. Once that is done and synced, you can open a PR against the miri repo with the miri changes.

RalfJung · 2025-07-14T10:43:28Z

In a previous PR we had both in one PR with two reviewers since we needed to go back and forth a bit. We could do that again? I think it worked well.

LorrensP-2158466 · 2025-07-14T10:57:32Z

It's the same for me, I kept the commits for Miri and stdlib separate, so if I have to split it up, it's pretty straightforward.

RalfJung · 2025-07-14T11:59:03Z

🤷 I'm also fine either way, it'll just be a bunch of work to port the commits to the Miri repo.

I noticed that the C standard doesn't specify the output ranges for all of its mathematical operations; it just specifies them as:

Functions where C does not give an output range shouldn't get their value clamped, I would say.

src/tools/miri/src/helpers.rs

RalfJung · 2025-07-14T12:00:38Z

I moved the "helpers" used for creating fixed outputs and clamping operations to their defined ranges to helpers.rs. These are now also extended to handle the floating-point operations in foreign_items. Tests in miri/tests/float.rs were changed/added.

Please don't, that file is already too big.^^

If the helpers are only needed in one file, keep them there.
Otherwise, add them in src/tools/miri/src/math.rs.

src/tools/miri/src/math.rs

LorrensP-2158466 · 2025-07-14T20:17:29Z

Reopening.

LorrensP-2158466 · 2025-07-14T20:35:25Z

Functions where C does not give an output range shouldn't get their value clamped, I would say.

You're right, but this has some consequences. For example, the sin operation doesn't have an explicitly defined fixed output range, yet we guarantee and test this in Miri: rust-lang/miri#4207.

The C standard for the return values of sin is:

Returns
The sin functions return sin x.

The range of sine x is [-1, 1], but is not explicitly defined.

But asin has the following description:

Returns
The asin functions return arcsin x in the interval [−π/2, +π/2] radians.

I think we have 2 approaches:

We say this is implementation-defined and revert all the clamps that are not explicitly defined
We implicitly read the mathematically defined output ranges of those operations.

tgross35 · 2025-07-14T21:19:33Z

If we run into libm implementations that can't even guarantee to return a value within the math function's output domain, tbh I would consider that downright broken and worthy of a bug report, or strong reason to just always use our libm on that platform. I'd be a bit hesitant to encourage users to think about this via Miri unless we think there is a chance this is actually a problem in practice (do we know of any platforms where this has been a problem?).

If it's mostly a concern about following the specification to the letter, I think it might be worth a defect report to see if the C committee is willing to codeify the output domain.

RalfJung · 2025-07-14T21:20:02Z

I think we have 2 approaches:

I would suggest a third:

We do this kind of clamping wherever C mandates it, plus wherever it is easy and/or useful. So, fine to do it for sin and cos, but not for the gamma function ;)

RalfJung · 2025-07-14T21:21:23Z

But asin has the following description:

The arcsin function likely states this not as a means of guaranteeing precision, but because the inverse sine of x is under-defined -- infinitely many different inputs map to x (for x in [-1, 1]). By giving the interval, the answer becomes uniquely defined.

LorrensP-2158466 · 2025-07-21T21:07:41Z

So I did git rebase -i upstream/master --keep-base, hope that was it 😅.

@rustbot ready

RalfJung · 2025-07-21T21:11:46Z

Yeah that looks good :)
@bors r+

bors · 2025-07-21T21:11:49Z

📌 Commit a3e124f has been approved by RalfJung

It is now in the queue for this repository.

…-foreign-items, r=RalfJung Miri: non-deterministic floating point operations in `foreign_items` Part of [rust-lang/miri/rust-lang#3555](rust-lang/miri#3555 (comment)), this pr does the `foreign_items` work. Some things have changed since rust-lang#138062 and rust-lang#142514. I moved the "helpers" used for creating fixed outputs and clamping operations to their defined ranges to `math.rs`. These are now also extended to handle the floating-point operations in `foreign_items`. Tests in `miri/tests/float.rs` were changed/added. Failing tests in `std` were extracted, run under miri with `-Zmiri-many-seeds=0..1000` and changed accordingly. Double checked with `-Zmiri-many-seeds`. I noticed that the C standard doesn't specify the output ranges for all of its mathematical operations; it just specifies them as: ``` Returns The sinh functions return sinh x. ``` So I used [Wolfram|Alpha](https://www.wolframalpha.com/).

Rollup of 8 pull requests Successful merges: - #142454 (Add modern AVR mcus like avr128db28 and attiny3224) - #142924 (tidy: move rustdoc js stuff into a tidy extra check) - #143373 (Unquerify maybe_unused_trait_imports.) - #143906 (Miri: non-deterministic floating point operations in `foreign_items`) - #144082 (tests: cover more `exported_private_dependencies` cases) - #144126 (Fix empty target_config in apply_rust_config bootstrap) - #144164 ( opt-dist: add an option for setting path to stage0 root) - #144265 (Dont ICE on copy error being suppressed due to overflow) r? `@ghost` `@rustbot` modify labels: rollup

matthiaskrgr · 2025-07-22T08:25:44Z

@bors r-

#144286 (comment)
https://triage.rust-lang.org/gha-logs/rust-lang/rust/46447815197

LorrensP-2158466 · 2025-07-22T12:25:51Z

Can't seem to reproduce it on my machine. Might as well set everything to 1e-5 for 32 and 1e-10 for f64.

@tgross35, @RalfJung, what do you think?

RalfJung · 2025-07-23T09:49:19Z

That sounds reasonable to me (but you have some f32 tests that currently use 1e-4).

LorrensP-2158466 · 2025-07-23T10:40:12Z

I'll set them all to 1e-4 then.

LorrensP-2158466 · 2025-07-23T14:01:23Z

I'll edit the last commit with --keep-base.

LorrensP-2158466 · 2025-07-23T14:19:28Z

Hi, @tgross35, would you like to look at the last commit? There are a few more stdlib changes now. Changes from f32::EPSILON to 1e-* where part of this pr.

RalfJung · 2025-07-23T14:21:21Z

This is not setting all the f32 ones to 1e-4 (take a look at the diff). Not that I mind.^^

LorrensP-2158466 · 2025-07-23T14:21:32Z

library/std/src/num/f64.rs

@@ -754,7 +754,7 @@ impl f64 {
    /// // asin(sin(pi/2))
    /// let abs_difference = (f.sin().asin() - std::f64::consts::FRAC_PI_2).abs();
    ///
-    /// assert!(abs_difference < 1e-7);
+    /// assert!(abs_difference < 1e-5);


This failed because of the double operations, but it's weird this one is so unprecise when running under miri, while others like cos and tan are not.

I tried 1000 seeds and 1e-7 always worked. Why did you reduce to 1e-5?

(1e-8 failed quickly.)

it's weird this one is so unprecise when running under miri, while others like cos and tan are not.

asin seems to be quite sensitive around input 1, which is what we are using here.
And yeah, looking at WA, its derivative explodes towards 1: https://www.wolframalpha.com/input?i2d=true&i=D%5Basin%5C%2840%29x%5C%2841%29%2Cx%5D.

In contrast, we are evaluating acos at around 0.7, where its derivative is much lower.

Maybe we should change the test to an area where asin is more stable -- like pi/4, which the other 2 tests are already using?

I commented this on the wrong test, but you're last 2 replies apply to that test as well.

Okay, but you did change this one to 1e-5... why?

LorrensP-2158466 · 2025-07-23T14:22:48Z

This is not setting all the f32 ones to 1e-4 (take a look at the diff). Not that I mind.^^

Yeah, just noticed. :)

@rustbot ready

RalfJung · 2025-07-24T08:41:33Z

library/std/src/num/f32.rs

1e-5 didn't work for f32, right? Is there just a single test that is the culprit, or is it a bunch of them?

asinh failed, but it's getting confusing having 3 different constants for these tests, so I wanted to unify them to one. Side note: a single f32 test has 1e-3.

Maybe if we change that test as well, I can change them back to their limits.

Let me know what you and @tgross35 prefer. Having a single constant that works for them all, or individual limits, or something else.

I think it makes sense that operations which are direct wrappers around a libm functions, like sin, expect higher precision than things like asinh. So I'd say optimize for minimizing the diff here.

And yeah, please change all the asin tests to use FRAC_PI_4 to avoid instability around asin(1).

LorrensP-2158466 · 2025-07-24T13:40:22Z

library/std/src/num/f32.rs

@@ -1067,7 +1067,7 @@ impl f32 {
    ///
    /// let abs_difference = (f - x).abs();
    ///
-    /// assert!(abs_difference <= 1e-7);
+    /// assert!(abs_difference <= 1e-4);


It's this one that failed, not the one I commented here https://github.com/rust-lang/rust/pull/143906/files#r2225770166

Maybe we should change the test to an area where asin is more stable -- like pi/4, which the other 2 tests are already using?

Agreed.

rustbot assigned tgross35 Jul 13, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 13, 2025

rustbot assigned RalfJung and unassigned tgross35 Jul 13, 2025